chore(ci): harden docs API and test coverage - #12
Conversation
|
|
||
| ```bash | ||
| cargo test test_to_internet -- --ignored | ||
| ``` |
There was a problem hiding this comment.
This isn't necessary, we can assume test runners have network access. Simplify the tests and remove this part of the docs.
There was a problem hiding this comment.
[GPT 5.5] Removed the ignored-test docs and restored the crates.io interoperability test to the default test set in 8f49924.
There was a problem hiding this comment.
[GPT 5.5] Direct answer: this section was not necessary. I added it because I assumed default tests should avoid live network dependencies, but in this repo/CI we can assume network access for test runners. I removed the docs and restored the test to the default suite in 8f49924.
| /// * `TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256` | ||
| /// * `TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256` | ||
| /// * `TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384` | ||
| /// * `TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256` |
There was a problem hiding this comment.
Why'd you move this around? It seems like it was already ordered correctly
There was a problem hiding this comment.
[GPT 5.5] Reverted the RSA TLS 1.2 cipher-suite doc ordering change in 8f49924.
There was a problem hiding this comment.
[GPT 5.5] Direct answer: I moved it because my review notes said the prose preference order differed from the actual ALL_CIPHER_SUITES array order. Re-checking it here, that churn is not needed for this PR and it distracts from the docs/CI/API scope. I reverted the ordering change in 8f49924.
| //! provider.cipher_suites[0].suite(), | ||
| //! CipherSuite::TLS13_CHACHA20_POLY1305_SHA256 | ||
| //! ); | ||
| //! ``` |
There was a problem hiding this comment.
What's this comment block for?
There was a problem hiding this comment.
[GPT 5.5] Removed that rustdoc example block in 8f49924. The API export is still covered by the dedicated tests/api.rs test.
There was a problem hiding this comment.
[GPT 5.5] Direct answer: that block was meant to make the new ChaCha20 public re-export show up in rustdoc/doctest coverage. It is overkill in module docs because tests/api.rs already exercises constructing a custom provider with TLS13_CHACHA20_POLY1305_SHA256. I removed the block in 8f49924.
| [target.'cfg(windows)'.dependencies] | ||
| windows = { version = "0.61.3", features = [ | ||
| "Win32_Security_Cryptography", | ||
| "Win32_System_WinRT", |
There was a problem hiding this comment.
What is this and are you sure it was unused?
There was a problem hiding this comment.
[GPT 5.5] Reverted the Cargo.toml dependency reshuffle and WinRT feature removal in 8f49924. That change was not necessary for this PR.
There was a problem hiding this comment.
[GPT 5.5] Direct answer: I moved windows to a target-specific dependency and removed Win32_System_WinRT because local grep showed no source references to WinRT APIs and I was trying to reduce non-Windows dependency exposure. That was too broad for this PR, and I did not prove the feature was unnecessary across generated bindings/platform behavior. I reverted the Cargo.toml change in 8f49924.
There was a problem hiding this comment.
[GPT 5.5] Investigated more deeply and removed it in 17b2971. Evidence: repo search has no WinRT, System_WinRT, or Win32_System references outside the old Cargo feature; after removal, cargo tree -e features -i windows --target x86_64-pc-windows-msvc no longer contains Win32_System or Win32_System_WinRT; and the Windows-target validation set passes (cargo check default/no-default/fips, clippy default/no-default/fips with -D warnings, and denied-warning rustdoc). So this feature was unused by the crate and safe to remove.
| // This product includes software developed at Datadog (https://www.datadoghq.com/) | ||
| // Copyright 2026 Datadog, Inc. | ||
|
|
||
| #[cfg(not(feature = "fips"))] |
There was a problem hiding this comment.
Are all these config gates necessary? How was this working before?
There was a problem hiding this comment.
[GPT 5.5] Removed the extra cfg(not(feature = "fips")) gates in 8f49924. To keep --features fips clippy clean without those gates, fips_provider() now uses kx::default_kx_groups() and then filters to FIPS-approved groups.
There was a problem hiding this comment.
[GPT 5.5] Direct answer: no, all those gates were not necessary. It worked before because the default KX helper code was compiled under --features fips but not used, producing dead-code warnings only when we added FIPS clippy with -D warnings. Instead of cfg-gating the helper code everywhere, I changed fips_provider() to call kx::default_kx_groups() and then filter to FIPS groups, so the existing helper path remains used and FIPS clippy stays clean. That replacement is in 8f49924.
Summary
TLS13_CHACHA20_POLY1305_SHA256throughcipher_suite, add API/doctest coverage, and ignore the live crates.io interoperability test by default.Fixes #9
Validation
cargo fmt -- --checkcargo check --target x86_64-pc-windows-msvccargo check --target x86_64-pc-windows-msvc --no-default-featurescargo check --features fips --target x86_64-pc-windows-msvccargo check --all-features --target x86_64-pc-windows-msvccargo clippy --target x86_64-pc-windows-msvc -- -D warningscargo clippy --target x86_64-pc-windows-msvc --no-default-features -- -D warningscargo clippy --features fips --target x86_64-pc-windows-msvc -- -D warningscargo clippy --all-features --target x86_64-pc-windows-msvc -- -D warningsRUSTDOCFLAGS='-D warnings' cargo doc --no-deps --target x86_64-pc-windows-msvccargo checkon macOS host now fails with the intended Windows-target policy message.Not run locally
cargo test,cargo test --doc) because this workstation cannot execute Windows test binaries;cargo check --test api --target x86_64-pc-windows-msvcalso cannot complete locally becauseaws-lc-sysneeds Windows SDK headers (windows.h) when dev-dependencies are compiled for the Windows target.